Skip to content

fix: Test Manifest, parameterized do not override sibling failures - #15831

Merged
josevalim merged 4 commits into
elixir-lang:mainfrom
PragTob:exunit-paramaterized-test-rerun-stick-with-2-tuple
Sep 2, 2026
Merged

fix: Test Manifest, parameterized do not override sibling failures#15831
josevalim merged 4 commits into
elixir-lang:mainfrom
PragTob:exunit-paramaterized-test-rerun-stick-with-2-tuple

Conversation

@PragTob

@PragTob PragTob commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Fixes #15820

Alternative approach to #15825.

In short, parameterized tests aen't accounted for in the test manifest, so if a variant first failed it could get overridden by a later run with different parameters as the key is just {module, test}.

Based on feedback, this PR tries to solve this by keeping all parameterized tests marked as failing.
So for variants 1, 2, 3, 4 ran and only 2 failed, all 4 get rerun which isn't too much of a cost.
For this small cost, we can keep the format of the manifest file, as it was before.

There is one wrinkle to this, which is I couldn't find a good way to stop the override ("do not delete a failing test from the manifest, if I have the same id") while still ever deleting a failing test (which we need, otherwise mix test --failed would be ever growing) other than to track which tests failed this run. So, we only refuse to clear it if we know the test failed this run.

The additional work to track is quite minimal though.

It adds an additional argument to put_test though, which I decided to give a default to not increase the splash radius of the PR. Worth potentially removing, unless we think it's fair game for external callers.

Fixes elixir-lang#15820

Alternative approach to elixir-lang#15825.

In short, parameterized tests aen't accounted for in the test manifest,
so if a variant first failed it could get overridden by a later run
with different parameters as the key is just {module, test}.

Based on [feedback](elixir-lang#15825 (comment)),
this PR tries to solve this by keeping _all_ parameterized tests marked
as failing.
So for variants 1, 2, 3, 4 ran and only 2 failed, all 4 get rerun which isn't
too much of a cost.
For this small cost, we can keep the format of the manifest file,
as it was before.

There is one wrinkle to this, which is I couldn't find a good way
to stop the override ("do not delete a failing test from the manifest,
if I have the same id") while still ever deleting a failing test
(which we need, otherwise `mix test --failed` would be ever growing)
other than to track which tests failed _this run_. So, we only
refuse to clear it if we know the test failed _this run_.

The additional work to track is quite minimal though.

It adds an additional argument to `put_test` though, which I
decided to give a default to not increase the splash radius of the
PR. Worth potentially removing, unless we think it's fair game
for external callers.
@josevalim

Copy link
Copy Markdown
Member

@PragTob thank you! I slightly changed the code to keep the passed and failures separately in the manifest. I also removed the integration test since the unit test gives us enough coverage!

@josevalim
josevalim merged commit 9360782 into elixir-lang:main Sep 2, 2026
15 checks passed
@josevalim

Copy link
Copy Markdown
Member

💚 💙 💜 💛 ❤️

@PragTob

PragTob commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

👌 Makes sense to keep in the failure manifest, should have seen that. Thanks!
IMG_20180222_184826

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

failing parameterized tests may not be retried by mix test --failed

2 participants